-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[gearbox] Add peer gbsyncd for swss if gearbox exists #10504
Conversation
can you document how is this change tested? |
can you kill asic sync or gbsync and will all three docker restarted? |
Added tests for them |
# Add gbsyncd to FEATURE table, if not in. It did have same config as syncd. | ||
if [ -z $($SONIC_DB_CLI CONFIG_DB HGET 'FEATURE|gbsyncd' state) ]; then | ||
local CMD="local r=redis.call('DUMP', KEYS[1]); redis.call('RESTORE', KEYS[2], 0, r)" | ||
$SONIC_DB_CLI CONFIG_DB EVAL "$CMD" 2 'FEATURE|syncd' 'FEATURE|gbsyncd' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jimmyzhai , @abdosi ,
shouldn't sonic-db-cli here query the global feature list but not the namespace.
This code adds the feature in namespace but the global feature list does not have the gbsyncd entry.
This does not fix the container_checker requirement which looks into the global feature table.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docker syncd/gbsyncd is per-namespace. Should we have syncd/gbsyncd entry at per-namespace feature table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure which code use the per-namespace entry for features but container_checker script only looks into global namespace for feture state and generates the per-namespace instance name based on the feature name from a global list. So I think gbsyncd also needs to be added to global list.
"show feature state" list the feature from global space and I do not see any namespace option for this CLI.
@abdosi , please comment. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature table entry is present both in global and namespace config db. Idea is the entry value will always be same and consistent across global and State DB.
When user enable/disable feature state using cli hostcfgd update value both global and namespace specific db's.
And when we do show commands we get the db value from global DB's and display them.
@jimmyzhai I agree with @anamehra can we update global Feature Table also when we are are doing for namespace so that table is consistent ?
Also with this change we will not need this #10834
cc @arlakshm
Why I did it
Fix #10501
Fix #9733
If having gearbox, we need
How I did it
How to verify it
Which release branch to backport (provide reason below if selected)
Description for the changelog
Link to config_db schema for YANG module changes
A picture of a cute animal (not mandatory but encouraged)